feat: Slack Avatar integration #27849
Conversation
github-actions
bot
added
risk:db-migration
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27849 +/- ##
==========================================
- Coverage 69.83% 59.98% -9.85%
==========================================
Files 1920 1921 +1
Lines 75242 75284 +42
Branches 8423 8423
==========================================
- Hits 52546 45160 -7386
- Misses 20635 28062 +7427
- Partials 2061 2062 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
the
github_actions
github-actions
bot
removed
the
github_actions
mistercrunch
force-pushed
the
avatar_url
branch
2 times, most recently
from
77c5a6a to
1162ff7
Compare
superset/views/users/api.py
Outdated
| ) | ||
| user_attrs.avatar_url = avatar_url | ||
| db.session.add(user_attrs) | ||
| db.session.commit() |
There was a problem hiding this comment.
by the book, a GET should not change any state. What about creating a celery beat job that would collect all the existing user avatars?
There was a problem hiding this comment.
This is the only element I did not fully address because it's effectively the feature here. Note that mutations will be very very sparse here. Mutation only happen if:
- feature is on (
SLACK_ENABLE_AVATAR+SLACK_API_TOKEN) - it's the very first time the avatar is requested for a given target user in the whole environment, doing a tiny update of UserAttributes.avatar_url once with a normal-sized string
In all normal day-to-day
- upon a new browser requesting an avatar, the GET does as a normal get (a one cell database lookup)
- 301 gets cached
- target image gets cached
mistercrunch
force-pushed
the
avatar_url
branch
2 times, most recently
from
4d5a9d0 to
9a412fc
Compare
Our current Avatars are a little dry, using a limited variety of colors
and showing the user's initials. The app could use a little more
personality.
Now given we already have a slack integration and administrators can
configure their `SLACK_API_TOKEN`, I was thinking we can use this
integration to get Slack's avatars integrated in Superset. Given the
popularity of Slack and users familiarity with the avatars there,
I thought it'd be neat to bring a bit more color in Superset.
The approach here is centered around a new endpoint
`/api/v1/user/{id}/avatar.png`, which fetches from a new column in
UserAttribute (our one-to-one extension to FAB's User model) for
avatar_url, and redirects to the destination. When loading for the
first time, if information is empty and Slack is configured, we ask
Slack for the URL, which is secret, but serves the image from outside
the authentication part of their API.
mistercrunch
force-pushed
the
avatar_url
branch
from
9a412fc to
376ac64
Compare
|
@dpgaspar I addressed pretty much all feedback. Personally prefer avoiding the CELERY_BEAT configuration, but open to consider that approach too if we decide it's preferable. |
| # under the License. | ||
| """empty message | ||
| Revision ID: bbf146925528 |
There was a problem hiding this comment.
What does this migration do? See that upgrade and downgrade are just pass
| # under the License. | ||
| """empty message | ||
|
|
||
| Revision ID: 0dc386701747 |
There was a problem hiding this comment.
Same as above, what would this migration do?
There was a problem hiding this comment.
Hey, sorry about the confusion, but in alembic the migration "chain" can split and you end up with multiple heads, typically when you rebase and there's been some other migration. This migration merges the 2 heads. In theory it's possible to do the equivalent of a rebase, but that requires a bit of work. In this case I "git rebased" twice during the lifecycle of this PR and had to merge the heads 2 times, leaving this 2 empty-ish migrations.
There was a problem hiding this comment.
more about it here -> https://alembic.sqlalchemy.org/en/latest/branches.html
|
I've tried enabling this in 4.1.0rc2, but I get the same behaviour as this issue #28833 I think it is caused by the combination of websockets with this feature. |
mistercrunch
added
🏷️ bot
SUMMARY
Our current Avatars are a little dry, using a limited variety of colors and showing the user's initials. The app could use a little more personality. Now given we already have a slack integration and administrators can configure their
SLACK_API_TOKEN, I was thinking we can use this integration to get Slack's avatars integrated in Superset. Given the popularity of Slack and users familiarity with the avatars there, I thought it'd be neat to bring a bit more color in Superset.Note that I'm also introducing the config
SLACK_ENABLE_AVATARSwhich isFalseby default, and added a note toUPDATING.mdfor release managers to switch it on if desired.The approach here is centered around a new endpoint
/api/v1/user/{id}/avatar.png, which fetches from a new column inUserAttribute (our one-to-one extension to FAB's User model) name
avatar_url, and redirects (301) to the destination. When loading for the first time, if information is empty and Slack is configured, we call Slack for the avatar URL, which has a secret location, but serves the image from outside the authentication part of their API once known. If the feature is off or no avatar url is set for the user, we simply return a204(no-content), preventing from logging errors or anything negative as this is expected.The
avatar_urlfield could be used programmatically in environments to serve other urls, but at this time I did not implement a GUI for users to self-set their avatar for instance. I think in most cases people will want to integrate with some external service (MSFT Team, Discord, ...). The approach I took here could be expanded to work with Gravatar for instance.About Perf
This approach while unconventional (is it?) should perform very well. Say in an environment with 1000 users, the Slack API should trigger once-ish for each user in the database to fill the
avatar_urlfor that user (1000 api hits). For each user navigating the website, a single hit to/api/v1/users/{some_user_id}/avatar.pngwhere the database is looked up is sufficient (up to 1M hits, but super low-key endpoint), after which the permanent redirect is cached client-side. The 204 should get cached like any other request following the configured cache-control in place.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF